Fix SQLi + XXE CVEs (CVE-2026-82583, -78224, -82578) with regression tests - #441
jonbartels wants to merge 3 commits into
Conversation
f263cd7 to
5b8b1bd
Compare
|
@abhinavagarwal07 - OpenIntegrationEngine is a fork of Mirth Connect. OIE is often affected by the same historical security risks as OIE. We learned about your security findings at https://abhinavagarwal07.github.io/posts/nextgen-mirth-connect-sqli-xxe/ Would you be willing to evaluate our fixes against your findings please? |
There was a problem hiding this comment.
🟡 Changes recommended
XML namespace behavior regresses, and the SQL injection test can falsely pass on MariaDB.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Hardens JDBC metadata, XSLT, and XML batch processing against SQL injection and XXE vulnerabilities, with regression coverage.
Changes:
- Adds allowlist validation for JDBC
selectLimit. - Secures XSLT and XML batch parsing.
- Adds unit/smoke tests and CI database configuration.
File summaries
| File | Description |
|---|---|
smoketest/.../base-vm-noop.xml |
Adds security-test channel fixture. |
smoketest/.../XsltStepXxeTest.java |
Tests XSLT XXE rejection. |
smoketest/.../XmlBatchXxeTest.java |
Tests XML batch entity rejection. |
smoketest/.../SecurityChannels.java |
Builds security-test channels. |
smoketest/.../OieServer.java |
Adds security-test server helpers. |
smoketest/.../DatabaseConnectorSqlInjectionTest.java |
Tests JDBC SQL injection blocking. |
smoketest/build.gradle |
Adds test compile dependencies. |
server/.../XsltStepSecurityTest.java |
Verifies generated XSLT hardening. |
server/.../XsltStep.java |
Secures generated transformer factories. |
server/.../XMLBatchAdaptor.java |
Uses hardened XML parsing. |
server/.../DatabaseConnectorServlet.java |
Validates selectLimit. |
ci/run-configuration.sh |
Supplies database test parameters. |
ci/harness.compose.yml |
Forwards harness JVM options. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@jonbartels Sure. I will review it. |
mgaffigan
left a comment
There was a problem hiding this comment.
I'm not sure about the database connector. XML and XsltStep look legitimate. The tests need to be substantially simplified:
- The SQL test should be a unit test - does not need a live server to confirm that it validates
- The Xxe repros should be fixture tests. See https://github.com/OpenIntegrationEngine/engine/blob/main/ci/README.md#add-a-fixture-test or https://github.com/OpenIntegrationEngine/engine/tree/main/ci/tests/110-hl7-no-op/channels/01-hl7-no-op
| export OIE_HARNESS_OPTS="-Doie.db.driver=org.postgresql.Driver -Doie.db.url=jdbc:postgresql://db:5432/mirthdb -Doie.db.user=mirthdb -Doie.db.password=mirthdb" | ||
| ;; | ||
| *-mysql) | ||
| export OIE_HARNESS_OPTS="-Doie.db.driver=com.mysql.cj.jdbc.Driver -Doie.db.url=jdbc:mysql://db:3306/mirthdb -Doie.db.user=mirthdb -Doie.db.password=mirthdb" | ||
| ;; | ||
| *) | ||
| export OIE_HARNESS_OPTS="" | ||
| ;; |
There was a problem hiding this comment.
These should come from the configurations/ directory, not hard-coded in this file.
| # The client SDK deserializes model objects with XStream, whose reflective converters need the | ||
| # same module access the server's own test task opens (server/build.gradle). Without at least | ||
| # java.base/java.util opened, deserializing a SortedSet response (e.g. the JDBC connector's | ||
| # getTables result) fails with InaccessibleObjectException on JDK 17. | ||
| jvm_opts=( | ||
| --add-exports=java.base/com.sun.crypto.provider=ALL-UNNAMED | ||
| --add-opens=java.base/java.util=ALL-UNNAMED | ||
| --add-opens=java.base/java.lang=ALL-UNNAMED | ||
| --add-opens=java.base/java.lang.reflect=ALL-UNNAMED | ||
| --add-opens=java.base/java.text=ALL-UNNAMED | ||
| --add-opens=java.sql/java.sql=ALL-UNNAMED | ||
| --add-opens=java.xml/com.sun.org.apache.xalan.internal.xsltc.trax=ALL-UNNAMED | ||
| ) |
There was a problem hiding this comment.
They should be the same as the runtime oieserver - not the other tests, I would presume.
| # Split the extra -D flags (set per configuration in run-configuration.sh) into an array so they | ||
| # reach the java command as separate arguments without unquoted globbing/word-splitting. | ||
| read -ra harness_opts <<< "${OIE_HARNESS_OPTS:-}" |
There was a problem hiding this comment.
Can we avoid? If the options come from a file, I should assume this is not required.
| public SortedSet<Table> getTables(String channelId, String channelName, String driver, String url, String username, String password, Set<String> tableNamePatterns, String selectLimit, Set<String> resourceIds) { | ||
| // Reject any selectLimit that is not one the server itself configured, before it can be | ||
| // executed as SQL (CVE-2026-82583). Done outside the try below so it is not re-wrapped. | ||
| validateSelectLimit(selectLimit); |
There was a problem hiding this comment.
Is this actually an issue? The whole point of this connector is to run arbitrary queries against a database, and I've not heard anything of an auth bypass. If we're always validating against the constant from the driver, why is it a parameter? What's the intended use of the parameter?
| script.append("tFactory.setFeature(Packages.javax.xml.XMLConstants.FEATURE_SECURE_PROCESSING, true);\n"); | ||
| script.append("try { tFactory.setAttribute(Packages.javax.xml.XMLConstants.ACCESS_EXTERNAL_DTD, ''); } catch (e) {}\n"); | ||
| script.append("try { tFactory.setAttribute(Packages.javax.xml.XMLConstants.ACCESS_EXTERNAL_STYLESHEET, ''); } catch (e) {}\n"); |
There was a problem hiding this comment.
If we expect these to succeed, we should not be swallowing all errors.
| // SPDX-License-Identifier: MPL-2.0 | ||
| // SPDX-FileCopyrightText: 2026 Open Integration Engine | ||
|
|
||
| package org.openintegrationengine.smoketest; |
There was a problem hiding this comment.
This does not seem to require any detail of a particular online database - presumably we can run this as a unit test.
abhinavagarwal07
left a comment
There was a problem hiding this comment.
Reviewed at 42db3da5 — notes inline.
The XXE fixes look right for the default JDK factory, and the batch fix covers Element_Name and Level as well as XPath_Query, which is broader than what was reported.
Three things I wanted to check: the selectLimit allowlist is populated from an API-writable source, the XSLT hardening is dropped silently on Saxon 9.x, and the batch parser is no longer namespace-aware. Measurements are in the inline comments — all run standalone on JDK 21, none against a live OIE server.
| addSelectLimits(allowedSelectLimits, DriverInfo.getDefaultDrivers()); | ||
|
|
||
| try { | ||
| addSelectLimits(allowedSelectLimits, configurationController.getDatabaseDrivers()); |
There was a problem hiding this comment.
The allowlist is populated from configuration the same caller can write.
PUT /api/server/databaseDrivers takes a caller-supplied selectLimit. It's annotated DATABASE_DRIVERS_EDIT, but stock DefaultAuthorizationController.isUserAuthorized() returns true unconditionally (:40-45) and is the only implementation in the tree. The value lands in the config property getDatabaseDrivers() reads first, ahead of dbdrivers.xml and the defaults (DefaultConfigurationController.java:753, :685).
So: PUT the payload as a driver's selectLimit, replay it here, reach executeQuery at :178.
Am I reading the stock authorization path right? If so, deriving selectLimit server-side from driver would avoid depending on it.
| * disable the check (fail closed). | ||
| */ | ||
| private void validateSelectLimit(String selectLimit) { | ||
| if (StringUtils.isBlank(selectLimit)) { |
There was a problem hiding this comment.
isBlank here and isEmpty at :160 differ. A whitespace-only selectLimit skips validation and then takes the query branch, trimming to "" and erroring into the fallback, so nothing executes. Is the difference deliberate?
| // are not resolved. setAttribute is guarded because some implementations (e.g. Saxon) reject | ||
| // these attributes; secure processing alone still applies. Mirrors XmlProcessor.configureSecureTF. | ||
| script.append("tFactory.setFeature(Packages.javax.xml.XMLConstants.FEATURE_SECURE_PROCESSING, true);\n"); | ||
| script.append("try { tFactory.setAttribute(Packages.javax.xml.XMLConstants.ACCESS_EXTERNAL_DTD, ''); } catch (e) {}\n"); |
There was a problem hiding this comment.
These catch blocks drop the restrictions with no signal when a factory rejects them. Emitted sequence on JDK 21:
| Saxon | FEATURE_SECURE_PROCESSING |
ACCESS_EXTERNAL_* |
source-document XXE |
|---|---|---|---|
| 9.5.1-5, 9.7.0-21, 9.9.1-8 | accepted | both throw IllegalArgumentException |
file read succeeds |
| 10.9, 11.6, 12.5 | accepted | accepted | blocked |
Secure processing succeeding on 9.x means nothing indicates the other two failed — it covers extension functions, not external document access.
useCustomFactory is supported and both tests set it false. In scope here? Failing closed when either attribute can't be set would cover it.
| // letting XPath.evaluate(InputSource) build its own DOCTYPE-resolving parser (XXE, | ||
| // CVE-2026-82578). getSecureDocumentBuilderFactory() already sets disallow-doctype-decl; | ||
| // the extra features below block external entities/DTDs and entity expansion outright. | ||
| DocumentBuilderFactory dbf = DocumentSerializer.getSecureDocumentBuilderFactory(); |
There was a problem hiding this comment.
DocumentBuilderFactory defaults namespaceAware to false; the previous XPath.evaluate(InputSource, ...) path parsed namespace-aware. On JDK 21:
//*[local-name()='message' and namespace-uri()='urn:test']— 1 match before, 0 after<batch xmlns="urn:test">splits to<message>hello</message>- prefixed input splits to
<p:message>hi</p:message>with noxmlns:p
Element_Name and Level serialize the same way, so it isn't limited to XPath_Query. dbf.setNamespaceAware(true) restored all three. Was this checked against the old path?
| dbf.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); | ||
| dbf.setXIncludeAware(false); | ||
| dbf.setExpandEntityReferences(false); | ||
| Document document = dbf.newDocumentBuilder().parse(new InputSource(bufferedReader)); |
There was a problem hiding this comment.
Separately: disallow-doctype-decl rejects every DOCTYPE, including internal-only DTDs that previously parsed. Worth a release note?
| @Test | ||
| void selectLimitDoesNotExecuteArbitrarySql() throws Exception { | ||
| Db db = Db.fromSystemProperties(); | ||
| assumeTrue(db != null, "oie.db.* coordinates not provided; skipping (embedded-database configuration)"); |
There was a problem hiding this comment.
This skips whenever oie.db.* is unset — the embedded-Derby configurations. That's the default deployment and the one the reported impact used (SYSCS_EXPORT_QUERY writing the channel table to an unauthenticated path). Deliberate?
| long messageId = server.submitMessage(channelId, payload, new LinkedHashMap<>()); | ||
|
|
||
| Status sourceStatus = awaitSourceStatus(server, channelId, messageId); | ||
| assertEquals(Status.ERROR, sourceStatus, "XSLT step resolved an external entity instead of denying " |
There was a problem hiding this comment.
Status.ERROR is also reached on a deploy or template failure, so it doesn't separate "external access denied" from "threw for another reason". Would a benign control plus asserting the file contents are absent work?
| String channelId = server.deployChannel(channel, "xml-batch-xxe"); | ||
| try { | ||
| String payload = "<?xml version=\"1.0\"?>" | ||
| + "<!DOCTYPE batch [<!ENTITY x \"" + MARKER + "\">]>" |
There was a problem hiding this comment.
Internal entity, so this covers expansion rather than external resolution. It passes because disallow-doctype-decl blocks both; under external-general-entities=false alone it'd fail while the file-read path stayed closed. Worth an external canary case?
| + "<batch><message>&x;</message></batch>"; | ||
| try { | ||
| server.submitMessage(channelId, payload, new LinkedHashMap<>()); | ||
| } catch (Exception batchRejected) { |
There was a problem hiding this comment.
Same as the SQLi test — a failure before XMLBatchAdaptor runs is indistinguishable from the fix working.
|
|
||
| String script = step.getScript(false); | ||
|
|
||
| assertTrue("secure processing should be enabled on the transformer factory", |
There was a problem hiding this comment.
This asserts the constant names appear in the generated string, so it passes on the Saxon 9.x configuration noted in XsltStep.java. Would stubbing a factory that accepts FEATURE_SECURE_PROCESSING and rejects both attributes be a better fit?
tonygermano
left a comment
There was a problem hiding this comment.
As you rework this to address some of the other feedback, can you split it up into multiple commits? Probably one for any harness pre-work that needs to be done, and then 1 commit per CVE? It should be plainly obvious which changes are related to which fixes.
DatabaseConnectorServlet.getTables executed the caller-supplied selectLimit query parameter as SQL via Statement.executeQuery. selectLimit is only ever a driver-specific metadata-probe template (the metadata dialog sends the driver's own value), so it is now ignored and resolved server-side from the built-in DriverInfo list keyed by the JDBC driver class; anything unrecognised uses the safe DatabaseMetaData.getColumns() path. Resolving only from DriverInfo.getDefaultDrivers() -- never the API-writable configured driver list -- closes the injection regardless of the deployment's authorization model. resolveSelectLimit is static and covered by a unit test that needs no database or live server. Trade-off: an admin-configured custom (non-built-in) driver no longer gets its optimized probe and falls back to the generic getColumns() path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Jon Bartels <jonathan.bartels@gmail.com>
XsltStep builds a TransformerFactory inside generated JavaScript, so it was missed by the Java-side XML hardening elsewhere in the tree. The generated script now enables FEATURE_SECURE_PROCESSING and sets ACCESS_EXTERNAL_DTD / ACCESS_EXTERNAL_STYLESHEET to "", blocking external entity resolution in both the stylesheet and the source XML. These are not swallowed: a factory that rejects either attribute fails the transform rather than running with external access silently left open. Covered by a unit test on the generated script and a ci/tests fixture: an external-entity message errors, a well-formed control transforms. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Jon Bartels <jonathan.bartels@gmail.com>
XMLBatchAdaptor evaluated XPath directly over an InputSource, letting the XPath engine build its own DOCTYPE-resolving parser. It now parses with DocumentSerializer.getSecureDocumentBuilderFactory() (disallow-doctype-decl, external entities/DTD off, no entity expansion) and evaluates against the parsed Document. namespaceAware is set true to preserve the prior XPath path's namespace-aware parsing (namespace-uri()/prefix-sensitive split queries). Covered by a smoke test that deploys a batch-splitting channel and asserts a DOCTYPE entity is not expanded. It is a Java test rather than a fixture because a rejected batch surfaces as a submit-time exception the fixture runner cannot model. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Jon Bartels <jonathan.bartels@gmail.com>
42db3da to
b767e4e
Compare
|
Thanks all — this was substantive feedback and it changed the shape of the PR. I force-pushed a rework: the branch is now three per-CVE commits ( SQL injection (CVE-2026-82583) — reworked, not just re-tested. @mgaffigan's question ("why is it a parameter?") and @abhinavagarwal07's point about the allowlist source are the same problem from two sides, and both are right. I verified it: So
XSLT step XXE (CVE-2026-78224). Now fails closed — the XML batch XXE (CVE-2026-82578). Kept the hardened-DBF parse and added Behavior-change note: The two XXE fixes themselves are essentially unchanged from what you all endorsed. CI is green across all seven configurations. Re-requesting review — thanks again. |
Summary
Fixes three publicly-disclosed vulnerabilities (advisory) that are present in this tree (4.6.0 is below every upstream fix version). Each fix uses an idiom already established in this codebase, and each is covered by a regression test in the
ci/smoketestharness that is RED on the vulnerable code and GREEN with the fix.DatabaseConnectorServletselectLimitagainst configured driversXsltStepXMLBatchAdaptorDocumentBuilderFactory, then evaluate XPathThe fixes
CVE-2026-82583 — SQL injection
DatabaseConnectorServlet.getTablesconcatenated the caller-suppliedselectLimitquery parameter into SQL and ran it viaStatement.executeQuery. The Database connector metadata dialog only ever sends aselectLimitdrawn from the configured driver list (DatabaseReader/DatabaseWriter→DriverInfo.getSelectLimit()), soselectLimitis now validated against that list (getDatabaseDrivers()plus the built-inDriverInfodefaults, always included so a cleared list cannot disable the check) before any SQL runs. Non-allowlisted values are rejected with a generic exception that does not reflect the input; a blank value still routes to the safeDatabaseMetaData.getColumns()path. This mirrors the allowlist-at-a-single-choke-point approach of #361.Deliberately out of scope (follow-ups): the endpoint's
@MirthOperationhas nopermissionandauditable = false, anddriver/urlare unconstrained (SSRF / arbitrary class load). The same no-permission/auditable=falsegap exists on every other connector test servlet (file/tcp/http/smtp/ws/jms) and is better handled as its own change.CVE-2026-78224 — XSLT step XXE
XsltStepbuilds aTransformerFactoryinside generated JavaScript, so it was never reached by the Java-side XML hardening elsewhere in the tree. The generated script now enablesFEATURE_SECURE_PROCESSINGand setsACCESS_EXTERNAL_DTD/ACCESS_EXTERNAL_STYLESHEETto""(thesetAttributecalls guarded for implementations that reject them), on both the normal and iterator code paths — blocking external entity resolution in both the stylesheet and the source XML.CVE-2026-82578 — XML batch adaptor XXE
XMLBatchAdaptorevaluated XPath directly over anInputSource, letting the XPath engine build its own DOCTYPE-resolving parser. It now parses withDocumentSerializer.getSecureDocumentBuilderFactory()(disallow-doctype-decl, external entities/DTD off, no entity expansion) and evaluates against the parsedDocument. Note the output path in this same file was already hardened; only the reader side was missed.Tests
XsltStepSecurityTest(server unit test, runs in./gradlew :server:test) — asserts the hardening is emitted into the generated script.DatabaseConnectorSqlInjectionTest,XsltStepXxeTest,XmlBatchXxeTest(smoketest harness) — drive the live server through theClientSDK:_getTableswith an injectedselectLimitwhose alias would surface as a column; asserts it never does. Runs on DB-backed configurations (oie.db.*coordinates wired for postgres/mysql viarun-configuration.sh+harness.compose.yml); skips embedded-Derby.file:///etc/hostnameexternal entity; asserts the source message reachesERROR.SecurityChannelsbuilds these channels from a base VM no-op fixture; newOieServerhelpers add the connector call, aChannel-object deploy, and a message-list accessor.Verification
./gradlew :server:test—XsltStepSecurityTestpasses; existingdatatypes.xml,DocumentSerializer, andjdbctests still pass.ci/runtests.sh alpine-temurin21-postgres) exercises all three integration tests against the real server. To observe the RED (CVE-present) baseline, revert the threeserver/src/mainfixes and re-run.🤖 Generated with Claude Code